Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix heap-buffer-overflow issue in redisvFormatCommad #1097

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Sep 1, 2022

A command with a faulty formatting string that lacks the conversion specifier (the type) results in a ASAN heap-buffer-overflow.
This was due to that strchr() matches on null-termination, which triggers a continuation of the string parsing.

Fixes #956 which uses formatting string # % (0x23,0x20,0x25,0x20) where the flag space comes after %,
i.e also a missing type specifier.

The alternative fix in #957 would make the added testcase return a command string (*1\r\n$0\r\n\r\n, len=10),
but since the same formatting string used in printf will also fail I think this is better.

A command with a faulty formatting string that lacks the
conversion specifier results in a ASAN heap-buffer-overflow.
This was due to that strchr() matches on null-termination,
which triggers a continuation of the string parsing.
@michael-grunder michael-grunder merged commit 1abe0c8 into redis:master Sep 1, 2022
@michael-grunder
Copy link
Collaborator

Merged, thanks @bjosv!

@kristjanvalur
Copy link
Contributor

kristjanvalur commented Sep 1, 2022

IMHO, the fix in #957 is also warranted. It is a prudent safety measurement, since it is hard to prove that this case will never be hit. One should never increment a zero-delimited string twice, without checking for the zero delimiter inbetween.

@bjosv bjosv deleted the fix-format-specifier-issue branch September 2, 2022 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

【fuzz】heap-buffer-overflow
3 participants